membuffer: implement ART with basic get/set #1451
Conversation
ti-chi-bot
bot
added
dco-signoff: yes
Signed-off-by: you06 <[email protected]>
Signed-off-by: you06 <[email protected]>
Signed-off-by: you06 <[email protected]>
Signed-off-by: you06 <[email protected]>
you06-pingcap
force-pushed
the
memdb/art-set-get
branch
from
3ebca27 to
f0a768d
Compare
| if uint64(t.Size()) > t.bufferSizeLimit { | ||
| return &tikverr.ErrTxnTooLarge{Size: t.Size()} | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func (t *ART) search(key artKey) (arena.MemdbArenaAddr, *artLeaf) { |
There was a problem hiding this comment.
We can documents its return values. Specifically, what value does it return when the key is not found?
internal/unionstore/art/art_node.go
Outdated
| return bytes.Equal(l.getKeyDepth(depth), key[depth:]) | ||
| } | ||
|
|
||
| func (l *artLeaf) setKeyFlags(flags kv.KeyFlags) arena.MemdbArenaAddr { |
There was a problem hiding this comment.
What is the purpose of this return value? Seems it unused.
internal/unionstore/art/art.go
Outdated
| copy(oldVal, value) | ||
| return true | ||
| } | ||
| t.size -= len(oldVal) |
There was a problem hiding this comment.
I suppose this line should be put outside of this function?
| } | ||
|
|
||
| depth := uint32(0) | ||
| prevDepth := 0 |
There was a problem hiding this comment.
Is there a reason that prevDepth must be int? Can it be unit32 as well to avoid type conversions?
Signed-off-by: you06 <[email protected]>
Signed-off-by: you06 <[email protected]>
| if leaf == nil { | ||
| return 0, tikverr.ErrNotExist | ||
| } | ||
| if leaf.vAddr.IsNull() && leaf.isDeleted() { |
There was a problem hiding this comment.
Why is the isDeleted used here but not in the above Get function? Or when should the isDeleted be used?
There was a problem hiding this comment.
isDelete marks the leaf is removed from the tree, which is used for cleanup after staging.
The difference between Get and GetFlags is the flag-only key (created by UpdateFlags), whose value address is null, so Get will return not exist error, meanwhile GetFlags should read the updated flags.
The RBT will remove the cleanup nodes from the tree, but ART will not (by now). Removing the node can reduces the height of the tree but also introduces the memory fragmentation (#1375). ART's performance isn't affected by the number of nodes, so it's ok to just mark it's deleted.
There was a problem hiding this comment.
Better to add comments about it here.
internal/unionstore/art/art.go
Outdated
| lcp := longestCommonPrefix(l1Key, l2Key, depth) | ||
|
|
||
| // calculate the common prefix length of new node. | ||
| an, n4 := t.newNode4() |
There was a problem hiding this comment.
Please keep the naming convention, there are places where newArtNode, newN4 := t.newNode4() is used.
For example using newArtNode or prevArtNode for ArtNode types, and node4Ptr for *node4 types.
| } | ||
| return newLeaf.addr, lf | ||
| } | ||
| if !valid && next.kind == typeLeaf { |
There was a problem hiding this comment.
What's the meaning of valid here? Does it mean there is a leaf node but it is empty?
There was a problem hiding this comment.
valid means if the current depth within the length of key. If valid is false, the in-place leaf is what we look for. If the in-place leaf is empty, we create it.
There was a problem hiding this comment.
Better to add commments about it at the valid function define location.
Signed-off-by: you06 <[email protected]> address comment Signed-off-by: you06 <[email protected]>
ti-chi-bot
bot
added
needs-1-more-lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cfzjywxk, ekexium The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
ref pingcap/tidb#55287
This PR implements ART with basic get and set.